-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[KOGITO-8961][KSW-Operator] Split use case and test examples #139
Conversation
b88f893
to
84489e3
Compare
@@ -31,11 +31,12 @@ import ( | |||
|
|||
func TestKogitoServerlessBuildController(t *testing.T) { | |||
namespace := t.Name() | |||
ksw := test.GetKogitoServerlessWorkflow("../config/samples/"+test.KogitoServerlessWorkflowSampleYamlCR, namespace) | |||
ksw := test.GetBaseServerlessWorkflowNoPackage(namespace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What package means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some test that needs as a path "../config/samples/" to load the yaml's files, others "../../config/samples/" because they are inside a package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use a smarter way of doing that. Maybe we can infer the path we are and compose it dynamically. If you think it is worth a shot, it would be a great improvement. I wouldn't do it if it takes too much time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricardozanini is it ok for your this https://github.com/kiegroup/kogito-serverless-operator/pull/139/files#diff-8affcd5838f425d5db07c12738684112559a18621ec539e9b33986abe804db7fR124-R132 in conjunction with the runtime.Caller ? No other ideas to decide the path, but I'm open if you have better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment here: https://github.com/kiegroup/kogito-serverless-operator/pull/139#discussion_r1213170430
😆
0e2d838
to
91f326f
Compare
@@ -1,10 +0,0 @@ | |||
apiVersion: sw.kogito.kie.org/v1alpha08 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't we discuss the we wouldn't change the config/samples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have agreed the proposal of @davidesalerno to use the needed yaml files and create the structs with the few fields needed in the tests instead to have one yaml file for each test, this file is replaced by the platform plus the field with the base image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config/samples
directory is used by the OLM/Operator SDK to compose the CSV. Meaning that users will see these examples on the OpenShift console.
Bear that in mind and keep only the simplest ones. I think this is the proposal, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we should keep here only the ones that will be in the CSV, one very simple example per custom resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, see here: https://operatorhub.io/operator/nexus-operator-m88i
Click on "View Example".
That's the purpose of this directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricardozanini Yep the file used by kustomization
https://github.com/kiegroup/kogito-serverless-operator/blob/main/config/samples/kustomization.yaml
are only:
sw.kogito_v1alpha08_kogitoserverlessworkflow.yaml
sw.kogito_v1alpha08_kogitoserverlessplatform.yaml
and in the folder in the PR we leave only these two and sw.kogito_v1alpha08_kogitoserverlessworkflow_devmodeWithConfigMapAndExternalResource.yaml
with a complete example, but we can move it in the docs folder
func GetPathForSamples(path string) string { | ||
operatorPath := strings.Split(path, "kogito-serverless-operator")[1] | ||
packages := strings.Count(operatorPath, "/") | ||
if strings.Contains(operatorPath, "/test/") || packages == 3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required, we can do it later. But maybe the filepath
package might help here? https://gobyexample.com/file-paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do later, is a func used only in test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to say this just now, but reading your comment here https://github.com/kiegroup/kogito-serverless-operator/pull/143#discussion_r1213619180 made me realize that we should move the resources to testdata
instead of tests
, so it won't be considered a go package by go tooling.
- Keep in
config/samples
the excerpt for the CSV as we discussed earlier - Use
testdata
instead oftests
for the unit tests within the project
307dc37
to
632740a
Compare
cbf8e6e
to
18ce8a8
Compare
2b1b589
to
0c2053c
Compare
Signed-off-by: desmax74 <mdessi@redhat.com>
See:https://issues.redhat.com/browse/KOGITO-8961
Description of the change:
Motivation for the change:
Checklist
How to backport a pull request to a different branch?
In order to automatically create a backporting pull request please add one or more labels having the following format
backport-<branch-name>
, where<branch-name>
is the name of the branch where the pull request must be backported to (e.g.,backport-7.67.x
to backport the original PR to the7.67.x
branch).Once the original pull request is successfully merged, the automated action will create one backporting pull request per each label (with the previous format) that has been added.
If something goes wrong, the author will be notified and at this point a manual backporting is needed.